Skip to content

feat(highlights): wire BibleReader to the highlights API (YPE-1034)#288

Open
cameronapak wants to merge 13 commits into
mainfrom
claude/consolidate-stacked-prs-bd457b
Open

feat(highlights): wire BibleReader to the highlights API (YPE-1034)#288
cameronapak wants to merge 13 commits into
mainfrom
claude/consolidate-stacked-prs-bd457b

Conversation

@cameronapak

@cameronapak cameronapak commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

BibleReader highlights read/write the real /v1/highlights API — now orchestrated by an explicit xstate v5 statechart — with a Sign in with YouVersion dialog, a just-in-time permission flow, recent colors, and staging-hardening fixes, all dark-launched behind the internal HIGHLIGHTS_LIVE flag (off; flip tracked in YPE-3874). Consolidates the YPE-1034 stack (#283, #285, #286, #287) into one reviewable PR, rebased onto main.

Start here: the flow is a state machine now — packages/ui/src/components/bible-reader-highlights-machine.ts, with a mermaid statechart in docs/highlight-flow-statechart.md. Paste the machine into the Stately visualizer to explore it interactively.

Changes

  1. xstate statechart (new this revision). The whole flow — auth branches, dialogs, redirect resume, serialized optimistic writes, overlay reconciliation — lives in one visualizable machine; useBibleReaderHighlights is a thin adapter. All previously-shipped invariants preserved (writes serialized FIFO, per-verse ownership tokens, per-sub-write settlement, apply collapses to range USFMs / remove DELETEs per verse, one refetch per settled batch, scope change drops the overlay).
  2. Sign in with YouVersion dialog (new). A signed-out color tap opens the ported Swift SignInWithYouVersionView (as a dialog, per web convention): INTRODUCING eyebrow, platform logo, optional integrator pitch from YouVersionPlatformConfiguration.signInPromptMessage, body with YouVersionPlatformConfiguration.appName, Yes Please / No Thanks. Confirm launches OAuth requesting highlights; decline keeps the verse selection. All 14 canonical string keys landed in en/es/fr (JIT + sign-out dialog copy included for future use).
  3. Flag-off is now truly off. With HIGHLIGHTS_LIVE off the color row and remove circles don't render at all (Copy/Share stay). Previously the row rendered with inert taps.
  4. Highlights persist to the user's YouVersion account with optimistic per-verse overlay; contiguous runs collapse to range USFMs on the wire.
  5. Failures route by cause: 401/403 invalidates the permission cache and re-prompts (applies only — a failed remove no longer re-prompts), network/5xx reverts the overlay (snackbar is YPE-3873), decline/cancel discards the pending highlight but keeps the selection.
  6. The color row shows the user's recent colors (GET /v1/highlights/recent-colors, fallback to the 5-color palette); active swatches show a checkmark.
  7. "Vapor" fix. A deleted highlight no longer flashes back when a stale read-replica fetch lands after the DELETE settled: remove-overlay entries are held until scope change / sign-out / a newer write instead of retiring on first reflection. (Same accepted trade-off as before: another device's same-verse edit renders stale until navigation or the next write.)
  8. Permission cache is user-scoped (review finding): stored as {userId, permissions}; user B can never read user A's grants, and a callback that doesn't echo granted_permissions can't resurrect stale ones.
  9. Callback URL cleanup is surgical (review finding): only data_exchange_status / granted_permissions are stripped; host app query params and the hash survive.
  10. useApiData fetches when enabled flips false→true, clears data on disable, and drops stale responses via latest-wins sequencing.

Flow

flowchart LR
    A[Tap color] --> B{Signed in +<br>highlights permission?}
    B -- yes --> C[Optimistic overlay + POST]
    B -- no session --> D[Sign-in dialog] -- Yes Please --> E[OAuth requesting highlights]
    B -- no permission --> F[JIT confirm dialog] -- Continue --> G[Data-exchange grant]
    D -- No Thanks --> H[Discard pending, keep selection]
    F -- Cancel --> H
    E --> I[Pending highlight resumes]
    G --> I
    I --> C
Loading

Full statechart: docs/highlight-flow-statechart.md

Breaking / Migration

Local-only highlights are gone: the localStorage store is deleted without migration per the server-only ADR — highlights are account data and un-render on sign-out.

Test plan

  • pnpm test: 893 passing (core 345, hooks 285, ui 263), pnpm typecheck and pnpm lint clean
  • New coverage: vapor-bug reproduction (failing-first), sign-in dialog flow, flag-off hides the color row, user-scoped permission cache (user switch without sign-out), surgical callback cleanup
  • Needs manual check before HIGHLIGHTS_LIVE flips: one live data-exchange round-trip on staging — callback params (data_exchange_status, granted_permissions) are assumed from spec; handling is isolated in buildDataExchangeUrl/parseDataExchangeCallback
  • The accepted stale-until-navigation trade-off (change 7) still needs product sign-off before the flag flips

Replaces the stacked drafts #283, #285, #286, #287.

🤖 Generated with Claude Code

Greptile Summary

This PR wires BibleReader highlights to the live highlights API behind a dark-launch flag. The main changes are:

  • A new state machine for highlight auth, dialogs, optimistic writes, and reconciliation.
  • Server-backed highlight reads and writes with recent colors and per-verse remove handling.
  • User-scoped permission caching for highlights grants.
  • Data-exchange callback parsing with surgical URL cleanup.
  • Flag-off behavior that hides highlight controls while keeping copy and share available.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.
  • The latest fixes cover the previously reviewed permission-cache, callback-cleanup, and pending-intent paths.

Important Files Changed

Filename Overview
packages/core/src/YouVersionPlatformConfiguration.ts Adds user-scoped storage for granted permissions and clears cached grants on sign-out.
packages/core/src/Users.ts Persists callback user info before seeding granted permissions from the sign-in return.
packages/core/src/data-exchange.ts Adds data-exchange token handling, callback parsing, grant caching, and targeted URL cleanup.
packages/ui/src/components/bible-reader-highlights-machine.ts Adds the state machine that owns highlight auth flow, optimistic writes, retry routing, and overlay reconciliation.
packages/ui/src/components/use-bible-reader-highlights.ts Connects React auth, highlight data, recent colors, and dialog controls to the highlights state machine.

Comments Outside Diff (2)

  1. packages/ui/src/components/use-bible-reader-highlights.ts, line 3837 (link)

    P1 Pending Intent Clears Too Early

    When the resumed highlight write fails after a sign-in or data-exchange return, this path has already deleted the pending highlight. A 401/403 or transient write failure then only logs and reverts, so the tap that started the auth flow is lost instead of being kept and re-prompted like the normal apply path.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: packages/ui/src/components/use-bible-reader-highlights.ts
    Line: 3837
    
    Comment:
    **Pending Intent Clears Too Early**
    
    When the resumed highlight write fails after a sign-in or data-exchange return, this path has already deleted the pending highlight. A 401/403 or transient write failure then only logs and reverts, so the tap that started the auth flow is lost instead of being kept and re-prompted like the normal apply path.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Fix in Claude Code Fix in Cursor Fix in Codex

  2. packages/ui/src/components/use-bible-reader-highlights.ts, line 3436-3437 (link)

    P2 Range Response Loses Verses

    If GET /v1/highlights ever returns a range passage such as JHN.3.16-18, parseInt maps it only to verse 16. The trailing verses are omitted from highlightedVerses, and the reconcile path using the same parsing can leave overlay entries for verses 17-18 pinned until navigation.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: packages/ui/src/components/use-bible-reader-highlights.ts
    Line: 3436-3437
    
    Comment:
    **Range Response Loses Verses**
    
    If `GET /v1/highlights` ever returns a range passage such as `JHN.3.16-18`, `parseInt` maps it only to verse 16. The trailing verses are omitted from `highlightedVerses`, and the reconcile path using the same parsing can leave overlay entries for verses 17-18 pinned until navigation.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Fix in Claude Code Fix in Cursor Fix in Codex

Reviews (4): Last reviewed commit: "docs(highlights): document intentional 5..." | Re-trigger Greptile

Context used (8)

  • Rule used - Unified versioning: All packages must maintain exa... (source)
  • Rule used - Zero React: Pure TypeScript, no React dependencies (source)
  • Rule used - Context and Provider in separate files, exported v... (source)
  • Rule used - Auto-injected styles: index.ts calls injectStyles(... (source)
  • Context used - CLAUDE.md (source)
  • Context used - AGENTS.md (source)
  • File used - docs/review-guidelines.md (source)
  • Context used - Enforce project-specific review guidelines - see d... (source)

cameronapak and others added 10 commits July 14, 2026 09:53
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dark-launch flag

Replace the localStorage highlight store (YPE-642 stand-in) with real API
wiring per YPE-1034 ADR-001: highlights are server-only account data, deleted
locally with no migration.

- New internal useBibleReaderHighlights seam: fetches the current chapter's
  highlights via useHighlights (chapter USFM passage_id, keyed on version),
  renders through an in-memory optimistic overlay, reverts + console.error on
  write failure (toasts and 401/403 handling are PR 2)
- Contiguous verse runs collapse to range USFMs on the wire (usfm-ranges.ts,
  mirroring the verse-share run-grouping idiom); colors sent as lowercase hex
- Internal HIGHLIGHTS_LIVE dark-launch flag (off, not exported from the
  package entry) gates fetches, writes, and rendering; setHighlightsLive() is
  the test/Storybook-only override
- Auth-gated: reads YouVersionAuthContext directly (now exported from hooks)
  so a missing auth provider degrades to signed-out instead of the useYVAuth
  throw; sign-out un-renders highlights immediately
- better-result added to the ui package for error typing at the new seam's
  write boundary; core's throwing clients are untouched

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… responses

Adversarial review of the highlights wiring surfaced one blocker and two
races, all rooted in useApiData:

- The fetch effect ignored enabled transitions: a hook that mounted disabled
  (auth still resolving) never fetched once enabled flipped true, so a
  signed-in reader rendered zero highlights until a write or navigation.
  enabled now rides alongside the caller-supplied deps.
- Disabling kept the last response, so stale account data could render across
  sign-out or a host-controlled auth user switch. Disabling now clears data
  and error.
- refetch-initiated requests escaped cancellation: a stale refetch for a
  previous chapter resolving late could clobber the new chapter's data. All
  requests now go through a monotonic sequence; only the latest-issued
  request may commit state.

In the BibleReader seam hook:

- Successfully settled writes now hand their verses to a confirmed set whose
  overlay entries are dropped when the post-write refetch lands, so server
  truth wins again instead of the optimistic entry masking later remote
  changes until navigation.
- Documented the two remaining apply/remove concurrency windows (in-flight
  POST vs DELETE ordering, snapshot revert vs concurrent write) at the write
  boundary; a real operation queue is deliberately PR 2.

New tests: useApiData enabled transitions and stale-response handling, and a
seam-hook integration test through the real useHighlights/useApiData path
(module-level mocks had hidden the enabled-flip bug).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Turn a color tap without a session or the `highlights` permission into an
applied highlight across the two auth paths, behind the internal
HIGHLIGHTS_LIVE flag.

Core: DataExchangeClient (POST /data-exchange/token, Zod-validated), the
hosted-grant URL builder + callback parser/handler, and an optimistic
permission cache on YouVersionPlatformConfiguration seeded from
`granted_permissions` on the sign-in and data-exchange callbacks (server
401/403 invalidates it). SignInWithYouVersionResult gains `permissions`.

Hooks: useHighlightAuthActions exposes one-fell-swoop sign-in (requesting
`highlights`), the just-in-time data-exchange redirect, permission
reads/invalidation, and the data-exchange return handler.

UI: useBibleReaderHighlights runs the state machine — pending highlights
persist to sessionStorage (~10-min expiry) across the redirect round-trip and
apply on a granted return; a permission confirm dialog (copy matched to the
native SDK, en/fr/es) gates the grant. Write failures route by status
(401/403 re-prompts and keeps pending; 5xx/network reverts and discards).
Apply/remove writes are serialized through a FIFO queue with per-verse
ownership, closing the two concurrency windows PR1 documented. Copy/share-only
behavior with no auth provider is unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…on hydrates

The resume effect consumed the data-exchange return status into a local, but
bailed on `!isAuthenticated` first. The shipped YouVersionAuthProvider
hydrates userInfo asynchronously, so the first effect run after a redirect
return is always unauthenticated: the status was consumed and lost, and when
the session flipped the cancel/failure branch was skipped — the pending
highlight survived a decline and the just-declined dialog re-opened.

Discard the pending highlight at consume time for any non-granted return,
before the auth gate: it runs exactly once and needs no session (a decline
kills the intent regardless of who signs in).

Tests: the cancel-return test previously set signedIn=true synchronously — a
timing the real provider never produces — which hid the bug. It now mounts
signed out and flips the session after mount (failed against the old code,
passes now), plus a failure-status variant with the same timing and the
granted-return test aligned for realism.

Also document the three review-deferred follow-ups (expired-token 401
misrouting, resume-write failure handling, remove-failure re-prompt) at their
code sites and in the changeset.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…watch

Render the server's recent-colors list in BibleReader's verse action
popover, and swap the active/remove swatch's X for a checkmark to match
iOS (platform-sdk-swift #179). Still behind the internal HIGHLIGHTS_LIVE
flag (YPE-1034 PR3).

- useBibleReaderHighlights fetches getRecentColors once the feature is
  live (flag on + auth provider + signed in) and exposes recentColors;
  it resets to null on sign-out / flag-off and on fetch failure so the
  row falls back to the default palette.
- VerseActionPopover takes an optional recentColors prop: normalized
  (lowercase, strip leading #, drop non-6-hex), deduped
  first-occurrence-wins, never reordered (server order is truth), and
  horizontally scrollable on overflow. Falls back to HIGHLIGHT_COLORS
  when recents are absent/empty. Active colors outside the palette still
  get a remove circle.
- HighlightColor widened from the 5-literal union to string (loosening,
  non-breaking); HIGHLIGHT_COLORS still exported as the default palette.
- New icons/check; ADR-005 as-built notes updated.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-ups on the recent-colors popover change:

- The row's new `overflow-x: auto` forces overflow-y out of `visible`
  (CSS spec), clipping the swatches' focus-visible ring (ring-2 +
  offset-2, ~4px overpaint) and hover scale-110 — including for prod
  users with HIGHLIGHTS_LIVE off, since the popover itself is not
  flag-gated. Add 6px padding with compensating negative margin so the
  overpaint stays inside the scroll box without changing layout, and a
  regression test guarding the padding/overflow pairing.
- Rework the primary recent-colors integration test to the production
  auth timing: mount signed out (as YouVersionAuthProvider hydrates
  async), assert no fetch, then flip signed-in and assert the fetch
  fires — the sync signed-in mount shape both prior PRs were dinged for.
- Document the deferred account-switch-without-sign-out staleness at the
  fetch effect (recents keyed on `live` only, not user identity).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- ui: default button variant now bg-primary/text-primary-foreground
  (was white-on-white in light theme, making the permission dialog's
  Continue button invisible). YouVersionAuthButton pins bg-background so
  its brand surface is unchanged.
- ui: retire the optimistic overlay only once a fetch reflects the write,
  so read-after-write lag no longer flickers a highlight out/back (apply)
  or resurrects a removed one (remove).
- hooks/ui: stop refetching per write; the seam hook coalesces to one
  refetch per settled batch.
- core/ui: send one DELETE per verse instead of a range (range delete
  is unsupported server-side); docstring updated to stop claiming it.
- core styles: fade highlight fills in/out (~250ms, reduced-motion aware).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…re too

An adversarial review caught a regression from the refetch coalescing: on
ANY failure the whole batch's overlay reverted and no refetch fired, so
sub-writes that succeeded server-side vanished from the UI (apply) or
resurrected as ghosts (per-verse remove) until the next navigation/write.

- runApply / runRemove / the resume-path write now track each sub-write's
  outcome: succeeded runs/verses register for reconciliation (overlay holds
  until a fetch reflects them), only truly-failed verses revert.
- Exactly one refetch per batch, success or failure, restoring the
  self-correction the per-write auto-refetch used to provide.
- Integration tests for partial apply failure, partial remove failure
  (no ghosts), and all-fail; the overlay-retirement test now distinguishes
  retired from still-masking via a divergent server color.
- Changeset notes the partial-failure semantics and the accepted
  concurrent-edit staleness trade-off (pending product sign-off).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Jul 14, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: cf593b1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@youversion/platform-react-hooks Patch
@youversion/platform-react-ui Patch
@youversion/platform-core Patch
vite-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment thread packages/core/src/YouVersionPlatformConfiguration.ts
Comment thread packages/core/src/data-exchange.ts Outdated
@cameronapak

Copy link
Copy Markdown
Collaborator Author

There's one more issue where the highlight after being deleted comes back for split seconds, like a vapor, and then disappears.

Isn't life like a vapor?

@cameronapak

Copy link
Copy Markdown
Collaborator Author

One remaining thing that the flow misses is a dialog when you're not signed in when tapping a highlght color:

Mimic the Swift sign-in sheet shows (SignInWithYouVersionView.swift)

Bottom sheet, 80% height, drag indicator, themed to the reader. Top to bottom:

"INTRODUCING" eyebrow caption
YouVersion Platform logo (image asset, not text)
Optional integrator message from YouVersionPlatformConfiguration.signInPromptMessage (the app's own pitch line, hidden if unset)
Body: "{appName} wants to connect to your YouVersion Bible App account. This will allow them to see and interact with your Bible App activity. Would you like to proceed?"
"Yes Please" (primary, 300pt pill) → closes sheet, launches browser OAuth (PKCE) requesting profile + highlights
"No Thanks" (secondary) → dismiss only
The artifact now has a visual mockup of this plus a table of all 14 string keys (sign-in sheet, JIT permission alert, both sign-out alerts) with the English copy — that's the verbatim set for React Web per the meeting's "pull copy from Swift" action item. Note the JIT and sign-out dialogs are plain native alerts; only sign-in is a sheet.
SCR-20260715-kkgf

This info would go in a dialog, not a bottom sheet on React Web SDK

@cameronapak

Copy link
Copy Markdown
Collaborator Author

One potential issue is... we want to disable highlights action bar and actions by default, unless the user specifies they want it. The highlights action bar right now goes as such: If HIGHLIGHTS_LIVE is true, then it's going to call the real API. If HIGHLIGHTS_LIVE is false, then right now it allows highlights but in localStorage. The issue... If HIGHLIGHTS_LIVE is false, then our expected behavior is that we don't want the action bar to show up at all and highlights to be available at this time. Right?

…view fixes

- Rewrite the highlights flow as an xstate v5 machine (bible-reader-highlights-machine.ts);
  useBibleReaderHighlights is now a thin adapter. Statechart doc in
  docs/highlight-flow-statechart.md.
- Add SignInWithYouVersion dialog (Swift sheet ported as a dialog) shown on a
  signed-out color tap, with all 14 canonical i18n keys (en/es/fr) and new
  appName/signInPromptMessage config.
- Fix the "vapor" bug: remove-overlay entries no longer retire on reflection,
  so a stale replica read can't resurrect a deleted highlight.
- Flag-off now hides the highlight UI entirely (color row + remove circles).
- Scope the granted-permissions cache to the signed-in user (Greptile P1).
- Data-exchange callback cleanup now strips only its own params (Greptile P2).
- Remove-failure 401/403 no longer re-prompts the permission dialog.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@cameronapak

Copy link
Copy Markdown
Collaborator Author

All three items addressed in def70d3, plus the flow is now an xstate v5 statechart per our discussion (full rewrite — nothing is live yet, so this was the moment to structure it right).

Vapor bug — root cause confirmed with a failing-first reproduction: the reconcile step retired a remove-overlay entry as soon as any fetch reflected the deletion, so a later stale read-replica response containing the highlight repainted it for a frame. Fix: remove-overlay entries never retire on reflection — a removed verse stays suppressed until scope change, sign-out, or a newer write re-claims it. Same accepted trade-off the PR already documents. "You are a mist that appears for a little while and then vanishes" — the highlight, however, now stays gone.

Sign-in dialog — ported from SignInWithYouVersionView as a dialog (not a bottom sheet, per your note): INTRODUCING eyebrow, platform logo, optional integrator pitch from the new YouVersionPlatformConfiguration.signInPromptMessage, body paragraph with the new appName config, Yes Please / No Thanks pills. A signed-out color tap now opens it instead of redirecting straight to OAuth; decline keeps the verse selection. All 14 canonical string keys from the Swift reference landed in en/es/fr (the JIT + sign-out copy included so localization syncs once).

Flag-off behavior — confirmed and implemented: with HIGHLIGHTS_LIVE off, the color row and remove circles don't render at all (Copy/Share stay). There was no localStorage fallback left — the row was rendering with inert taps; now it's gone entirely.

The statechart is in docs/highlight-flow-statechart.md (mermaid), and the machine source pastes straight into the Stately visualizer. 893 tests green (core 345, hooks 285, ui 263), typecheck + lint clean.

Comment thread packages/ui/src/components/bible-reader-highlights-machine.ts Outdated
A pending highlight resumed after sign-in/data-exchange now routes write
failures through the same handling as a user-initiated apply: 401/403
invalidates the permission cache, re-stashes the pending highlight (in its
own scope), and re-opens the permission dialog instead of silently dropping
the user's tap. No unattended redirect loop: the dialog requires a click.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread packages/ui/src/components/bible-reader-highlights-machine.ts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@cameronapak

Copy link
Copy Markdown
Collaborator Author

For the record on Greptile's last finding ("Pending Intent Still Drops", now resolved): the 5xx-on-resumed-write behavior is intentional and documented in code as of cf593b1. Transient failures consume the highlight intent uniformly with user-initiated applies — after the redirect round-trip the user is signed in with the permission, so a re-tap just works, and the failure surfaces via the snackbar (YPE-3873). Re-stashing would auto-apply the highlight on a later mount within the 10-min TTL with no user action, which is a worse surprise than asking for one more tap. Flagging so product can override if silent-retry semantics are preferred.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant